Add per-element thresholds to adaptive zoom#409
Conversation
Signed-off-by: Ayoub LABIDI <ayoub.labidi@protonmail.com>
Signed-off-by: Ayoub LABIDI <ayoub.labidi@protonmail.com>
| return { | ||
| enabled: adaptiveTextZoom?.enabled ?? NadViewerParameters.ENABLE_ADAPTIVE_ZOOM_DEFAULT, | ||
| threshold: adaptiveTextZoom?.threshold ?? NadViewerParameters.THRESHOLD_ADAPTIVE_ZOOM_DEFAULT, | ||
| edgeSideLabelThreshold: |
There was a problem hiding this comment.
if adaptiveTextZoom?.edgeSideLabelThreshold not defined maybe set to adaptiveTextZoom?.threshold to keep previous usage of one threshold only
| edgeMiddleLabelThreshold: | ||
| adaptiveTextZoom?.edgeMiddleLabelThreshold ?? NadViewerParameters.THRESHOLD_ADAPTIVE_ZOOM_DEFAULT, | ||
| edgeMiddleArrowThreshold: | ||
| adaptiveTextZoom?.edgeMiddleArrowThreshold ?? NadViewerParameters.THRESHOLD_ADAPTIVE_ZOOM_DEFAULT, |
There was a problem hiding this comment.
| adaptiveTextZoom?.edgeMiddleArrowThreshold ?? NadViewerParameters.THRESHOLD_ADAPTIVE_ZOOM_DEFAULT, | |
| adaptiveTextZoom?.edgeMiddleArrowThreshold ?? NadViewerParameters.THRESHOLD_ADAPTIVE_TEXT_ZOOM_DEFAULT, |
There was a problem hiding this comment.
The const was already named like that, I preferred not to rename it, there is some work left to do on naming in general not just here, I think we should do it in a separate PR
| } | ||
| } | ||
| } | ||
| for (const edge of edges) { |
There was a problem hiding this comment.
| for (const edge of edges) { | |
| if (maxDisplayedSize > adaptiveTextZoom.edgeMiddleLabelThreshold) { | |
| for (const edge of edges) { |
There was a problem hiding this comment.
I reworked this part to do only one loop and I added the check for middleEdgeInfos
| this.redrawMiddleEdgeArrowAndLabels( | ||
| halfEdge1, | ||
| halfEdge2, | ||
| edgeInfo, | ||
| edgeInfoMetadata.direction ?? edgeInfoMetadata.directionB, | ||
| edgeInfoMetadata.directionA, | ||
| edgeInfoMetadata.labelA !== undefined && edgeInfoMetadata.labelB !== undefined | ||
| showLabel && edgeInfoMetadata.labelA !== undefined && edgeInfoMetadata.labelB !== undefined | ||
| ); |
There was a problem hiding this comment.
Manage case of calling redrawMiddleEdgeArrowAndLabels with showArrow=false ?
There was a problem hiding this comment.
do not call if both showLabel and showArrow are false
There was a problem hiding this comment.
I added the check on the direction to avoid doing extra work for nothing when showArrow is false
but the other case when showLabel and showArrow are false is already covered, we don't even reach this part of the code
rolnico
dismissed
their stale review
Some new comments have to be taken into account
|
sBouzols
left a comment
sBouzols
left a comment
There was a problem hiding this comment.
Code Review OK
Tests in demo app OK
Tests in GridSuite OK
Console warning check OK
3 participants
Please check if the PR fulfills these requirements
Does this PR already have an issue describing the problem?
No
What kind of change does this PR introduce?
This adds independent thresholds for edge side labels (edgeInfo1/edgeInfo2), the edge middle label, and the edge middle arrow (edgeInfoMiddle), so each can fade out at its own zoom level for smoother large-network rendering.
What is the current behavior?
Adaptive zoom used a single threshold to remove/recreate legends, edge infos and text nodes all at once.
What is the new behavior (if this is a feature change)?
Independent thresholds so each element can be added at its own zoom level for smoother large-network rendering.
Does this PR introduce a breaking change or deprecate an API?
If yes, please check if the following requirements are fulfilled
What changes might users need to make in their application due to this PR? (migration steps)
Group all adaptive zoom options into a single
adaptiveTextZoomobject on NadViewerParametersOptions (enabled, threshold, edgeSideLabelThreshold, edgeMiddleLabelThreshold, edgeMiddleArrowThreshold), replacing the previous flat optionsenableAdaptiveTextZoomandadaptiveTextZoomThresholdOther information: